feat: integrate with verification attempt events #226

zacharis278 · 2024-09-27T13:29:40Z

Description:

Adds support for handling events/signals on the new VerificationAttempt model added to edx-platform. This handling replaces this libraries support for reacting to changes on a SoftwareSecurePhotoVerification since those models are slated for deprecation -- openedx/edx-platform#35128.

Full details of this initiative can be found at: openedx/platform-roadmap#367

Decisions made in thisPR:

This is a breaking change that drops support for the integration with SoftwareSecurePhotoVerification. Existing records will continue to support a verified name but no new verified names will be created (or removed) based on that model. This also means any existing pending records will no longer be updated.
We will update https://github.com/openedx/edx-platform/blob/5043260bd6d9b92225389d0b7994cf78bffd42ee/lms/djangoapps/verify_student/management/commands/approve_id_verifications.py#L4 to explicitly update name affirmation for those records since the above decision has severed this connection.
This library will assume any IDV_ATTEMPT signal refers to the new VerificationAttempt model and update platform_verification_attempt_id. This has some impact on edx-platform. We used IDV_ATTEMPT_APPROVED in those events to trigger a certificate update from the SSOVerification and PhotoVerification models. I now believe that was incorrect. Those records do not get any of the other openedx events for IDV_ATTEMPT_* and more importantly should not trigger name affirmation here. The event to trigger a certificate calculation from an SSOVerification should be wholly separate from this event.

Impacts:

Releasing this becomes a breaking change that is a hard cutoff for SoftwareSecure IDV records. We will update the task to for manual approval of remaining SSO IDV records to explicitly update name affirmation rather than rely on signal handlers.
We need to cleanup the certification code so it listens to a edx-platform specific signal from SSOVerification records (the way it did before) in addition to IDV_ATTEMPT_APPROVED. SSO/PhotoVerification models should not emit the new openedx event. That work would block merging this.

feat: rework idv cert trigger openedx/edx-platform#35580

JIRA:

COSMO-493

Pre-Merge Checklist:

Updated the version number in edx_name_affirmation/__init__.py if these changes are to be released. See OEP-47: Semantic Versioning.
Described your changes in CHANGELOG.rst.
Confirmed Github reports all automated tests/checks are passing.
Approved by at least one additional reviewer.

Post-Merge:

Create a tag matching the new version number.

zacharis278 · 2024-09-27T13:47:25Z

edx_name_affirmation/handlers.py

@@ -35,56 +41,52 @@ def verified_name_approved(sender, instance, **kwargs):  # pylint: disable=unuse
        )


-def idv_attempt_handler(attempt_id, user_id, status, photo_id_name, full_name, **kwargs):


This is only ever triggered by a SoftwareSecurePhotoVerification and can removed since we will no longer be creating those records.

Why remove this?
We run idv_update_verified_name_task on an attempt id which is not unique between the two data models making it ambiguous as to what field the attempt id from the LMS is really for. While we could start passing two fields (type, id) throughout the service that's going to be messy and type serves no purpose once SoftwareSecurePhotoVerification support is deprecated.

Question: Do we need approval from Axim for this? I know we believe no one uses Software Secure outside of 2U, but if it's not deprecated, I'm a little concerned about breaking this flow. The Sumac release is being cut on 10/23/2024, so this will make it into that release before the Software Secure integration is removed. Do you have any concerns about this?

ahh this is a great point. I originally wrote this under the assumption this repo was not part of Open edX since 2U is the only one using it. But as you pointed out earlier this is a default dependency so maybe we need to reconsider here

to close out this thread. We've decided to move forward with this as is. Given the following:

name-affirmation is only used by 2U

name-affirmation can drop support for SoftwareSecurePhotoVerifications prior to it's full removal in the LMS

functionally this would work as it did before for any verifications using the new models

zacharis278 · 2024-09-27T13:54:21Z

edx_name_affirmation/tasks.py

@@ -43,7 +43,7 @@ def idv_update_verified_name_task(self, attempt_id, user_id, name_affirmation_st
    # want to grab all verified names for the same user and name combination, because
    # some of those records may already be associated with a different IDV attempt.
    verified_names = VerifiedName.objects.filter(
-        (Q(verification_attempt_id=attempt_id) | Q(verification_attempt_id__isnull=True))
+        (Q(platform_verification_attempt_id=attempt_id) | Q(platform_verification_attempt_id__isnull=True))


both here and with delete this library now assumes any future idv change is related to the new model, we've completely dropped support for SoftwareSecurePhotoVerification changes.

github-actions · 2024-09-30T14:17:05Z

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
edx_name_affirmation
__init__.py
handlers.py					72-73
tasks.py
edx_name_affirmation/tests
test_handlers.py
test_tasks.py
Project Total

_{This report was generated by python-coverage-comment-action}

zacharis278 · 2024-09-30T19:31:14Z

edx_name_affirmation/tasks.py

-        # otherwise if there are no entries, we want to create one.
+
+    # if there are no entries to update, we want to create one.
+    if not verified_names_updated:


This actually fixes what appears to be an existing bug. If a user has a previously verified name using a different method (eg proctoring) and an new IDV creation event is emitted a new verified name record is not created. The event just gets dropped. From a user standpoint this doesn't really happen since the UI will post a blank name before that event but it still seemed like a gap.

schenedx

The change LGTM

MichaelRoytman

Looks good to me! I left a few questions but nothing blocking.

MichaelRoytman · 2024-10-04T13:50:53Z

edx_name_affirmation/handlers.py

@@ -35,56 +41,52 @@ def verified_name_approved(sender, instance, **kwargs):  # pylint: disable=unuse
        )


-def idv_attempt_handler(attempt_id, user_id, status, photo_id_name, full_name, **kwargs):


Question: Do we need approval from Axim for this? I know we believe no one uses Software Secure outside of 2U, but if it's not deprecated, I'm a little concerned about breaking this flow. The Sumac release is being cut on 10/23/2024, so this will make it into that release before the Software Secure integration is removed. Do you have any concerns about this?

MichaelRoytman · 2024-10-04T13:56:18Z

edx_name_affirmation/handlers.py


+    log.info(f'IDV_ATTEMPT {status} signal triggering Celery task for user {user.id} '
+             f'with photo_id_name {event_data.name}')


Nit. Could we use this as an opportunity to rename photo_id_name? I have always found it a little confusing, and it's specific to Software Secure or forms of IDV that use IDs. Maybe "verified name" or something?

MichaelRoytman · 2024-10-04T14:03:55Z

edx_name_affirmation/tasks.py

    else:
        verified_names = VerifiedName.objects.filter(proctored_exam_attempt_id=proctoring_attempt_id)
        log_message['field_name'] = 'proctored_exam_attempt_id'
        log_message['attempt_id'] = proctoring_attempt_id

    if verified_names:
-        log.info(
+        log.info(  # there's a bug in this log message


What's the bug? Is this something we need to ticket to fix or just a note-to-self that should be removed?

ah this was a note to me to figure out if I wanted to fix this or ticket. The log message assumes the field the attempt_id is verification_attempt_id (now updated to platform_verification) but it could be any of those types. It's minor, given our timeline I'll just remove the comment and backlog this

zacharis278 commented Sep 27, 2024

View reviewed changes

zacharis278 force-pushed the zhancock/handle-idv-signals branch from 3cc89c6 to 278cc6e Compare September 30, 2024 12:55

zacharis278 added 7 commits September 30, 2024 12:31

feat: initial approach

d59f7e6

feat: unused status function

23c4243

fix: task logic

aacb6cd

feat: refactor existing tests for new IDV source

576f04c

test: new test case

81a41c3

feat: install openedx-events

932e817

style: isort

220af76

zacharis278 force-pushed the zhancock/handle-idv-signals branch from d6d6caf to 220af76 Compare September 30, 2024 16:31

zacharis278 added 4 commits September 30, 2024 12:45

feat: update version and changelog

010ced2

fix: create verified name if none exists

2596ec4

style: newline

c1ebde1

style: fix bad style on return statement

c0b5184

zacharis278 commented Sep 30, 2024

View reviewed changes

schenedx approved these changes Oct 4, 2024

View reviewed changes

MichaelRoytman approved these changes Oct 4, 2024

View reviewed changes

zacharis278 marked this pull request as ready for review October 4, 2024 17:08

style: review feedback

8e0d3d6

zacharis278 merged commit cef4654 into main Oct 4, 2024
6 checks passed

zacharis278 deleted the zhancock/handle-idv-signals branch October 4, 2024 17:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: integrate with verification attempt events #226

feat: integrate with verification attempt events #226

zacharis278 commented Sep 27, 2024 •

edited

Loading

zacharis278 Sep 27, 2024 •

edited

Loading

MichaelRoytman Oct 4, 2024

zacharis278 Oct 4, 2024

zacharis278 Oct 4, 2024

zacharis278 Sep 27, 2024

github-actions bot commented Sep 30, 2024 •

edited

Loading

zacharis278 Sep 30, 2024

schenedx left a comment

MichaelRoytman left a comment

MichaelRoytman Oct 4, 2024

MichaelRoytman Oct 4, 2024

MichaelRoytman Oct 4, 2024

zacharis278 Oct 4, 2024

		@@ -35,56 +41,52 @@ def verified_name_approved(sender, instance, **kwargs): # pylint: disable=unuse
		)


		def idv_attempt_handler(attempt_id, user_id, status, photo_id_name, full_name, **kwargs):


		log.info(f'IDV_ATTEMPT {status} signal triggering Celery task for user {user.id} '
		f'with photo_id_name {event_data.name}')

feat: integrate with verification attempt events #226

feat: integrate with verification attempt events #226

Conversation

zacharis278 commented Sep 27, 2024 • edited Loading

Decisions made in thisPR:

Impacts:

zacharis278 Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 30, 2024 • edited Loading

Coverage report

Choose a reason for hiding this comment

schenedx left a comment

Choose a reason for hiding this comment

MichaelRoytman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zacharis278 commented Sep 27, 2024 •

edited

Loading

zacharis278 Sep 27, 2024 •

edited

Loading

github-actions bot commented Sep 30, 2024 •

edited

Loading